Fix cross-route fragment scrolling - #163
Conversation
🦋 Changeset detectedLatest commit: 6736bcd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Reading up on this more, we should also cover |
6e34efb to
6736bcd
Compare
rschristian
left a comment
rschristian
left a comment
There was a problem hiding this comment.
I have a some doubts on the implementation but I see little issue with going forward with this.
The old impl was fairly obviously flawed but, IME, actually worked as expected the majority of the time, I believe due to scrollTo actually getting called early and natural browser scrolling kicking in after the route component actually fulled rendered. I think we'd need to wait for the current route to fully render before attempting to scroll programmatically if we want to be correct.
| Array.from(document.getElementsByName(fragment)).find( | ||
| element => element.localName == 'a' | ||
| ); |
There was a problem hiding this comment.
I don't know that we really need to support this, as it's super niche, but this can be done simpler:
| Array.from(document.getElementsByName(fragment)).find( | |
| element => element.localName == 'a' | |
| ); | |
| document.querySelector(`a[name="${fragment}"`); |
There was a problem hiding this comment.
I wanted to avoid potential CSS injection issues mainly
|
Agree, I do think that a full solution would require us to be integrated with data-fetching, ... so that we are aware of full route readiness. The current thing covers async routes and synchronous routes but not routes that load data and then render UI. For my tests in drydock this has been a step in the right direction though as before this hash based routing (i.e. scroll to my docs link) would never apply basically |
Ah, that's no good. Surprisingly the Preact docs site works pretty well (IME) at this, despite, with prefetching, most route swaps being synchronous. I'd expect the previous |
2 participants
Cross-route fragment links did not scroll to their target during app testing because the router reset pushed navigations to the top. This scrolls to the fragment after the destination route commits.